Add unit test to check app.booting flag - #97
Conversation
|
@bajtos Review please. |
There was a problem hiding this comment.
expect(process.bootFlags).to.include('booting')
There was a problem hiding this comment.
Please consider learning yourself more familiar with the different assertions provided by Chai, see http://chaijs.com/api/bdd/. The benefit of these assertions (compared to your solution above) is a much more helpful error message reported when the check fails.
There was a problem hiding this comment.
Will do. ;) Thanks for the suggestion.
51001a1 to
5449fdd
Compare
|
@bajtos Updated based on code review comments. Good to merge? |
|
@slnode test please |
|
There's one more thing I don't like about the current approach, sorry for realising it too late. By reusing Please rework the test to use different (new) flag, so that you don't have to modify any existing tests in this PR. |
|
And also please rebase on top of the current master to pull in the fix for the browserify-related test failures. |
5449fdd to
dba11ed
Compare
- Ensure `app.booting` is initially `undefined` - Ensure `app.booting` is set to true during boot execution - Ensure `app.booting` is set to false upon boot completion
dba11ed to
e89a60c
Compare
|
@bajtos Reworked. Review again please. |
|
@slnode test please |
There was a problem hiding this comment.
I am curious, why did you have to change .to.be.false() to .to.not.be.ok()?
There was a problem hiding this comment.
That was by accident actually. I looked up ok() and ...not.be.ok() and true()/false() makes more sense but I forgot to change it back. I will submit another PR to change it back before we forget about it.
|
LGTM. |
Add unit test to check `app.booting` flag
2 participants
Connected to #77